feat: refactor gateway api to operate on higher level semantics #176
Conversation
gateway/gateway.go
Outdated
| @@ -17,34 +16,110 @@ type Config struct { | |||
| Headers map[string][]string | |||
| } | |||
|
|
|||
| // API defines the minimal set of API services required for a gateway handler. | |||
| // ImmutablePath TODO: This isn't great, as its just a signal and mutable paths fulfill the same interface | |||
| type ImmutablePath interface { | |||
There was a problem hiding this comment.
Was matching the other path types, doesn't have to be though. This was mostly a placeholder
There was a problem hiding this comment.
(I've run out of time today, will try to do proper read tomorrow, for now dropping partial review with feedback on some TODOs and _redirects (i'd de-emphasize it, it is niche feature, can we move it back to the same place as ipfs-404 thingy? i got gut feeling entire thing will get way less noisy)
gateway/handler.go
Outdated
| logger.Debugw("serving ipns record", "path", contentPath) | ||
|
|
||
| i.addUserHeaders(w) // ok, _now_ write user's headers. | ||
| w.Header().Set("X-Ipfs-Path", contentPath.String()) // TODO: Should we even set this? |
There was a problem hiding this comment.
for completeness, yes
in practice we could skip these on verifiable responses and nobody would be impacted
(these matter only for implicit "deserialize for me pls" GET done for website hosting)
…eway's problem. Implemented blocks gateway example
|
Updated: pushed some updates here, still need to rebase on the gateway PRs that have happened in the meanwhile. There are still some TODOs to be figured out (and I think I found a bug in
I'd probably feel better about this if we could get kubo leveraging a blocks gateway implementation that lives here. How unreasonable of an approach is this? |
| func NewBytesFile(b []byte) File { | ||
| return &ReaderFile{"", NewReaderFile(bytes.NewReader(b)), nil, int64(len(b))} | ||
| return &ReaderFile{"", bytesReaderCloser{bytes.NewReader(b)}, nil, int64(len(b))} | ||
| } | ||
|
|
||
| // TODO: Is this the best way to fix this bug? | ||
| // The bug is we want to be an io.ReadSeekCloser, but bytes.NewReader only gives a io.ReadSeeker and io.NopCloser | ||
| // effectively removes the io.Seeker ability from the passed in io.Reader | ||
| type bytesReaderCloser struct { | ||
| *bytes.Reader | ||
| } | ||
|
|
||
| func (b bytesReaderCloser) Close() error { | ||
| return nil |
There was a problem hiding this comment.
This seems like a bug and a small fix for it. I might be missing something though about why it was done this way.
gateway/handler_defaults.go
Outdated
| } | ||
| defer data.Close() | ||
| case http.MethodGet: | ||
| gwMetadata, data, err = i.api.Get(ctx, imPath) |
There was a problem hiding this comment.
TODO: handle range requests, probably by copying the golang HTTP library's parser
# Conflicts: # gateway/gateway_test.go # gateway/handler.go # gateway/handler_block.go # gateway/handler_codec.go # gateway/handler_tar.go # gateway/handler_unixfs.go # gateway/handler_unixfs__redirects.go # gateway/handler_unixfs_dir.go # go.mod # go.sum
|
2023-02-28 conversation: |
Codecov Report
@@ Coverage Diff @@
## main ipfs/kubo#176 +/- ##
==========================================
- Coverage 48.13% 48.02% -0.11%
==========================================
Files 267 269 +2
Lines 32571 32982 +411
==========================================
+ Hits 15678 15841 +163
- Misses 15236 15483 +247
- Partials 1657 1658 +1
|
There was a problem hiding this comment.
Gave it another pass, commented inline. (Only this PR, I did not look at sharness yet, I assumed we want the dust to settle first)
Highlights:
- found some ways we could simplify+unify handling of path traversal errors
- we may want to introduce ContentType at top level APIs to avoid forcing unnecessary sniffing and block fetches when we have explicit one in root node or CID codec
- browser on often updated websites may hit an additional RTT around
If-None-Matchon /ipns/, but not sure if we should worry yet - general anxiety about huge refactor ;) 🫂
… the Gateway API implementer
|
2023-03-21 maintainer conversation: this is blocked from being merged until #206 lands. |
# Conflicts: # examples/gateway/common/blocks.go # examples/go.mod # examples/go.sum # gateway/errors.go # gateway/gateway.go # gateway/gateway_test.go # gateway/handler.go # gateway/handler_car.go # gateway/handler_codec.go # gateway/handler_test.go # gateway/handler_unixfs.go # gateway/handler_unixfs__redirects.go # gateway/handler_unixfs_dir.go # go.mod # go.sum
|
Blocked on Kubo sharness test passing in ipfs/kubo#9681 (needs rebase), which is blocked on ipfs/kubo#9746 landing. |
|
I rebased ipfs/kubo#9681 and everything is green 💚. |
Context: ipfs/boxo#215 We use commit from ipfs/boxo#176 (comment) to unblock work on ipfs-inactive/bifrost-gateway#61
Context: ipfs/boxo#215 Used commits from ipfs/boxo#176 (comment) and filecoin-saturn/caboose#68 to unblock work on #61
Context: ipfs/boxo#215 Used commits from ipfs/boxo#176 (comment) and filecoin-saturn/caboose#68 to unblock work on #61
There was a problem hiding this comment.
@hacdias @aschmahmann afaik this file is not used anywhere, but increases repo size by +14MB. 🙈
Too late to force-push main to fix it, but can we at least remove it in separate PR so there is no confusing examples/gateway/car-file-gateway/ ?
If we need fixtures for tests, we should always put them in testdata/ directory, which has special meaning in go tooling
There was a problem hiding this comment.
@lidel urgh, will do! Sorry missed that.
* refactor gateway api to operate on semantics to closer match user requests * rename gateway api interface to IPFSBackend * add blocks gateway implementation to the gateway package * refactored gateway examples * add default DNS resolvers * add default IPLD codecs --------- Co-authored-by: Henrique Dias <hacdias@gmail.com> Co-authored-by: Marcin Rataj <lidel@lidel.org>
This is a start at a gateway refactor so that we operate on higher level semantics rather than lower level ones ipfs/kubo#173.
This enables us to have all the gateway HTTP code separate from whether the data was retrieved over Bitswap, via requesting the entire result as a CAR file, etc.
This probably could've been done while still keeping a bunch of the traversal code in the gateway (and collecting metrics like first block times, etc.). However, IMO this seemed unnecessary given that it should be the job of the API implementations to verify the data is correct and the gateway code's job to "HTTP-ify" it and record some higher level metrics.
The result is the a lot of the code has been inverted, since instead of the order being "get a bit -> verify -> get more -> verify..." we have "resolve mutable -> do top level request and get async response -> process async response and emit the correct headers".
Given that the implementations (e.g. of the BlockGateway or the interface-go-ipfs-core one) aren't built yet this is all obviously still in flux, but wanted to put something up in case people think this approach is nuts 😅. For this to all be usable I suspect we'll probably need to have some shared utilities so we don't replicate some of the annoying gateway traversal code multiple times (e.g. handling _redirects).
Also addresses:
Requirements:
- [ ] ipfs/go-unixfs#142No longer needed.